-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enhancement] Introduce Link Target in Button Block #10128
Conversation
Nice fix, good consistency for URL input. It all works fine for me. One thing, the layout for me doesn't look the same, but this is also the same prior to your change, so unlikely something you introduced. The enter and menu ellipsis show on the next line, not all in a single line. I'm using Firefox on Linux if that makes a difference. Old version: New version: |
Thank you for the heads-up @mkaz ❤️ And I'm sorry about the delay in response. Thanks for testing the PR out. I believe the layout issue should be addressed in a different issue as it was already there previously. I'll try to check this out and send a PR for it as well. Thank you ❤️ |
Rather than continue the separate implementation of URL input in buttons, had you considered the recommendation in #8000 to consolidate to introduce the target option by using the same |
Thank you very much for pointing that out @aduth ❤️ I'll take another look into this and try to go forward using |
@nfmohit-wpmudev Any chance you can refresh this before the 4.1 UI freeze tomorrow? 😬 |
@chrisvanpatten I'll try my best, but I'm a bit confused about the refresh so I think this will need to survive another round 😞 Speaking of being confused, it'd be really helpful if I can have some insights about this from @aduth 🙏 if possible. So, I revisited the other blocks which have a URL Input (e.g. the Paragraph block) and found that they are all using the |
I can't speak for everyone but this feels like a case where we should get this version in, so it's in before UI freeze, and then iterate to use |
wouldn’t it be better if we created another |
What's the status of this one? would be good to get this small improvement in. |
At some point it would be great to increase code reuse between the component which edits link here and in the link popup that This would make it easier to keep those two in sync. |
This PR also seems to address #6392, which proposes changing the Button block to use the same popover as is used elsewhere. @gziolo There is a I think it's a good idea to refactor this PR to use it. I did start refactoring the other components into presentational components (so there's URLPopover, URLPopoverForm, URLPopoverInput, URLPopoverSubmitButton) a while ago, but didn't quite make it into a PR: The styles around the URL input are quite difficult to untangle. 😬 |
So there is another issue for that. Yes, that was my point. We should unify those two. It's a recurring issue people complain about. |
See also #12738 for an option that is growing on me. |
Thank you for the insights guys, I'll check out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking this PR as needs updates. Let us know when it's refreshed so we could review it 👍
Noting that updating to use |
PR refreshed. I've used |
@afercia - shipping something that quickly, or patching a release generally only happens if there's a regression. Does this PR contain any regressions? Of the four things you mentioned:
That's a fair criticism, but somewhat subjective and not a regression.
The current block has an apply button, but it doesn't actually do anything. As mentioned above, the submit behaviour of the form has been overridden. I consider it a bad experience (and a bug) in the current implementation that a URL can be entered and applied without the user having clicked the button. The behaviour in this PR is actually the same as the current button block, but it removes the confusing button.
I agree this would be great to add, but since it's not present on the current version of the button block it's a new feature rather than a regression.
Again this is a new feature, would love to see this addressed quickly though. |
@talldan thanks for the detailed recap. I'd like to remind everyone that any new code released in WordPress must be WCAG 2.0 compliant, according to the WordPress accessibility coding standards. Lack of user input validation and lack of proper feedback after an user action are specific SCAG violations, regardless of whether they're regressions 🙂 I'd really like to avoid to ship new code that's not accessible as it should be. It happened a bit too much often in the past and then it took months to be "improved'. |
@afercia It is really frustrating when that happens, and it can be hard to get a fix merged, I've seen that with a few of my PRs. At the same time, we've identified that these accessibility issues have been present in the button block from launch. When I search on github for any of the accessibility issues related to the button block that you've mentioned there are none, and that explains why they haven't been fixed. If we do want to get those things fixed, my belief is that it'd be beneficial to ship this first, and then tackle those issues individually. Why?
I'm ready and willing to work on one or two of the issues mentioned. But I'm not going to delay this PR any further by requesting further changes. I also believe it's unfair to @nfmohit-wpmudev to block his PR for even longer by asking him to start fixing issues he hasn't even caused. He's already put so much work into it! |
@talldan I guess you haven't found specific accessibility GitHub issues because the accessibility feedback was given directly on the issues and PRs during the button block implementation, and never addressed, unfortunately. As I commented above:
I have no objections to merging this PR. I do have objections with what gets actually shipped in a Gutenberg release though 🙂 Iterations can happen at any time, but non-accessible code should not be released. |
Oh no! Alright, lets make sure that doesn't happen this time. I'll merge this PR and then start making some issues in github so that those things are captured. No guarantees these things will be shipped in the next release, but as we're right at the start of the release cycle there's as good a chance as any. I'll also try to do my best to try to push those accessibility issues forwards and pick up at least one of the tasks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been through quite a bit of iteration over the last ten months. Thanks for sticking with it @nfmohit-wpmudev, massive kudos!
I think it's time to merge it.
There are definitely some existing issues (accessibility and otherwise) with the button block that this PR has resurfaced. Let's try to address them in follow-ups. I'll get to working on some issues to cover them.
Thank you so much for all your help @talldan ❤️ I look forward to joining on the left-over issues regarding this block so that we can make this perfect! Thank you again ❤️ |
Follow up issues:
(I'll edit this to add more as I create them) Also created a PR based on code review comment by @noisysocks, I've asked him to review it:
|
Great job here @nfmohit-wpmudev |
Me again @afercia 😄 I've created two issues here that I think cover your comments. Do let me know if I missed anything and I'll address it by updating the issues or creating other issues. I decided to create one issue, #16494 to cover the issue of link validation and screenreader feedback when adding a link, since I think they're related. #16495 covers the issues mentioned about the UI consistency. I think this one should also consider whether the settings should be in the sidebar (if the UI were made more consistent they wouldn't be in the sidebar). I had a scrollback to check for anything else. I think those two issues cover everything. |
Thank you so much @youknowriad ❤️ |
Thanks @nfmohit-wpmudev and @talldan ! I think all the pending items are covered in the new issues. |
Hey @partnuz I am checking with Gutenberg plugin version 9.2.1. Do a hard refresh. To make sure the cache is cleared. Then check again. |
Description
Similar PR: #12738
This PR closes #8000 which requests the addition of an option to open the button link in a new tab. This also updates the design of the link form to make it look similar to the RichText link modal.
How has this been tested?
This PR has been tested by going through the following steps:
Screenshots
Types of changes
This PR introduces a new
boolean
attribute namedlinkTarget
, which iftrue
, applies the_blank
target attribute. It addsToggleControl
within the form element of the button block link input, which is displayed via a toggle-able ellipsisIconButton
. The styles have been updated so that it looks similar to the link modal inRichText
.Checklist: